feat(orpc): add oRPC integration#344
Open
HugoRCD wants to merge 6 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Contributor
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces oRPC framework integration to evlog, adding an ChangesoRPC Integration Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Benchmark reportBundle size
|
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/orpc/src/ui.ts`:
- Around line 196-205: The syntaxHighlight function currently builds HTML from
raw JSON pieces and is later assigned to innerHTML, creating an XSS risk; update
syntaxHighlight (and where its output is set to innerHTML) to HTML-escape any
dynamic content (strings/keys/values) before wrapping them in span tags or,
better, build DOM nodes and set textContent for values and class names for
styling instead of concatenating HTML; ensure all occurrences of
string/key/number/boolean/null matches returned by syntaxHighlight are sanitized
(escape &, <, >, ", ') or replaced via textContent to eliminate script
injection.
In `@packages/evlog/src/orpc/index.ts`:
- Around line 11-13: Add JSDoc comments for the public exports EvlogOrpcOptions
and useLogger: for EvlogOrpcOptions (alias of BaseEvlogOptions) add a one-line
description explaining its purpose and any important fields/inheritance; for
useLogger add a short description of what it returns/does, document its
parameters and return type, and include any thrown errors or usage notes. Place
the comments immediately above the `export type EvlogOrpcOptions =
BaseEvlogOptions` and `export { useLogger }` lines so IDEs and generated docs
pick them up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a1ce7e61-8a34-42ac-80f9-290671a8da71
⛔ Files ignored due to path filters (2)
packages/evlog/test/toolkit/__snapshots__/api-surface.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.changeset/orpc-integration.md.github/pull_request_template.md.github/workflows/semantic-pull-request.ymlapps/docs/app/components/features/FeatureFrameworks.vueapps/docs/content/0.landing.mdapps/docs/content/1.start/3.installation.mdapps/docs/content/3.integrate/frameworks/00.overview.mdapps/docs/content/3.integrate/frameworks/15.orpc.mdapps/docs/skills/review-logging-patterns/SKILL.mdexamples/orpc/README.mdexamples/orpc/package.jsonexamples/orpc/src/index.tsexamples/orpc/src/ui.tsexamples/orpc/tsconfig.jsonpackage.jsonpackages/evlog/README.mdpackages/evlog/package.jsonpackages/evlog/src/orpc/index.tspackages/evlog/test/frameworks/orpc.test.tspackages/evlog/tsdown.config.ts
Comment on lines
+196
to
+205
| function syntaxHighlight(json) { | ||
| return JSON.stringify(json, null, 2) | ||
| .replace(/("(\\\\u[a-fA-F0-9]{4}|\\\\[^u]|[^\\\\"])*")(\\s*:)?/g, (match, str, _, colon) => { | ||
| if (colon) return '<span class="key">' + str + '</span>' + colon | ||
| return '<span class="string">' + str + '</span>' | ||
| }) | ||
| .replace(/\\b(true|false)\\b/g, '<span class="boolean">$1</span>') | ||
| .replace(/\\bnull\\b/g, '<span class="null">null</span>') | ||
| .replace(/\\b(-?\\d+\\.?\\d*([eE][+-]?\\d+)?)\\b/g, '<span class="number">$1</span>') | ||
| } |
Contributor
There was a problem hiding this comment.
Escape response content before writing to innerHTML (XSS risk).
syntaxHighlight() builds HTML from response data, and Line 239 injects it with innerHTML. If a response string contains HTML/script payload, it can execute in the page.
🔒 Proposed fix
+ function escapeHtml(value) {
+ return value
+ .replace(/&/g, '&')
+ .replace(/</g, '<')
+ .replace(/>/g, '>')
+ .replace(/"/g, '"')
+ .replace(/'/g, ''')
+ }
+
function syntaxHighlight(json) {
return JSON.stringify(json, null, 2)
.replace(/("(\\\\u[a-fA-F0-9]{4}|\\\\[^u]|[^\\\\"])*")(\\s*:)?/g, (match, str, _, colon) => {
- if (colon) return '<span class="key">' + str + '</span>' + colon
- return '<span class="string">' + str + '</span>'
+ const safe = escapeHtml(str)
+ if (colon) return '<span class="key">' + safe + '</span>' + colon
+ return '<span class="string">' + safe + '</span>'
})
.replace(/\\b(true|false)\\b/g, '<span class="boolean">$1</span>')
.replace(/\\bnull\\b/g, '<span class="null">null</span>')
.replace(/\\b(-?\\d+\\.?\\d*([eE][+-]?\\d+)?)\\b/g, '<span class="number">$1</span>')
}Also applies to: 239-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/orpc/src/ui.ts` around lines 196 - 205, The syntaxHighlight function
currently builds HTML from raw JSON pieces and is later assigned to innerHTML,
creating an XSS risk; update syntaxHighlight (and where its output is set to
innerHTML) to HTML-escape any dynamic content (strings/keys/values) before
wrapping them in span tags or, better, build DOM nodes and set textContent for
values and class names for styling instead of concatenating HTML; ensure all
occurrences of string/key/number/boolean/null matches returned by
syntaxHighlight are sanitized (escape &, <, >, ", ') or replaced via textContent
to eliminate script injection.
Comment on lines
+11
to
+13
| export type EvlogOrpcOptions = BaseEvlogOptions | ||
|
|
||
| export { useLogger } |
Contributor
There was a problem hiding this comment.
Add JSDoc for EvlogOrpcOptions and useLogger exports.
These are public APIs but currently undocumented in this module.
📝 Proposed fix
+/**
+ * Options accepted by `withEvlog()` for oRPC request instrumentation.
+ */
export type EvlogOrpcOptions = BaseEvlogOptions
+/**
+ * Access the current request-scoped logger outside oRPC context callbacks.
+ */
export { useLogger }As per coding guidelines, packages/evlog/src/**/*.{ts,tsx}: Add JSDoc comments on all public APIs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export type EvlogOrpcOptions = BaseEvlogOptions | |
| export { useLogger } | |
| /** | |
| * Options accepted by `withEvlog()` for oRPC request instrumentation. | |
| */ | |
| export type EvlogOrpcOptions = BaseEvlogOptions | |
| /** | |
| * Access the current request-scoped logger outside oRPC context callbacks. | |
| */ | |
| export { useLogger } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/evlog/src/orpc/index.ts` around lines 11 - 13, Add JSDoc comments
for the public exports EvlogOrpcOptions and useLogger: for EvlogOrpcOptions
(alias of BaseEvlogOptions) add a one-line description explaining its purpose
and any important fields/inheritance; for useLogger add a short description of
what it returns/does, document its parameters and return type, and include any
thrown errors or usage notes. Place the comments immediately above the `export
type EvlogOrpcOptions = BaseEvlogOptions` and `export { useLogger }` lines so
IDEs and generated docs pick them up.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/orpc/src/ui.ts (1)
199-204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape highlighted JSON tokens before writing to
innerHTML(XSS).At Line 242, untrusted response content is rendered via
innerHTML, andsyntaxHighlight()currently injects raw string/key tokens into HTML spans. This allows scriptable payloads in JSON strings.🔒 Proposed minimal fix
+ function escapeHtml(value) { + return value + .replace(/&/g, '&') + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, ''') + } + function syntaxHighlight(json) { return JSON.stringify(json, null, 2) .replace(/("(\\\\u[a-fA-F0-9]{4}|\\\\[^u]|[^\\\\"])*")(\\s*:)?/g, (match, str, _, colon) => { - if (colon) return '<span class="key">' + str + '</span>' + colon - return '<span class="string">' + str + '</span>' + const safe = escapeHtml(str) + if (colon) return '<span class="key">' + safe + '</span>' + colon + return '<span class="string">' + safe + '</span>' }) .replace(/\\b(true|false)\\b/g, '<span class="boolean">$1</span>') .replace(/\\bnull\\b/g, '<span class="null">null</span>') .replace(/\\b(-?\\d+\\.?\\d*([eE][+-]?\\d+)?)\\b/g, '<span class="number">$1</span>') }Also applies to: 242-242
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/orpc/src/ui.ts` around lines 199 - 204, The syntaxHighlight function is inserting raw JSON tokens into HTML, enabling XSS when that output is assigned to innerHTML; update syntaxHighlight (and any call sites that set innerHTML with its result) to escape HTML special characters (&, <, >, ", ') in the captured string/key tokens before wrapping them in spans or, alternatively, build DOM nodes and set textContent instead of interpolating raw strings, ensuring all occurrences of string and key values in syntaxHighlight are properly escaped before returning HTML.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/orpc/src/index.ts`:
- Around line 49-55: The middleware `authed` is declared `async` but doesn't
`await`, causing a `require-await` lint error; remove the `async` modifier on
the arrow function passed to `base.use` (i.e., change `async ({ context, next })
=> { ... }` to `({ context, next }) => { ... }`) while keeping the body intact
(creating the `AuthedUser`, calling `context.log.set`, and returning `next({
context: { ...context, user } })`) so the function still returns the Promise
from `next`.
---
Duplicate comments:
In `@examples/orpc/src/ui.ts`:
- Around line 199-204: The syntaxHighlight function is inserting raw JSON tokens
into HTML, enabling XSS when that output is assigned to innerHTML; update
syntaxHighlight (and any call sites that set innerHTML with its result) to
escape HTML special characters (&, <, >, ", ') in the captured string/key tokens
before wrapping them in spans or, alternatively, build DOM nodes and set
textContent instead of interpolating raw strings, ensuring all occurrences of
string and key values in syntaxHighlight are properly escaped before returning
HTML.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4f0c0cf0-b5bc-47f1-81f3-98278fe528d4
📒 Files selected for processing (4)
apps/docs/content/3.integrate/frameworks/15.orpc.mdexamples/orpc/README.mdexamples/orpc/src/index.tsexamples/orpc/src/ui.ts
Comment on lines
+49
to
+55
| const authed = base.use(async ({ context, next }) => { | ||
| const user: AuthedUser = { id: 'u-1', name: 'Alice', role: 'admin', apiKey: 'demo' } | ||
| context.log.set({ auth: { ok: true, userId: user.id, role: user.role } }) | ||
| return next({ | ||
| context: { ...context, user }, | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
Remove unnecessary async to satisfy require-await and keep lint green.
Line 49 declares an async middleware but does not await anything; this triggers the reported ESLint error.
✅ Minimal patch
-const authed = base.use(async ({ context, next }) => {
+const authed = base.use(({ context, next }) => {
const user: AuthedUser = { id: 'u-1', name: 'Alice', role: 'admin', apiKey: 'demo' }
context.log.set({ auth: { ok: true, userId: user.id, role: user.role } })
return next({
context: { ...context, user },
})
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const authed = base.use(async ({ context, next }) => { | |
| const user: AuthedUser = { id: 'u-1', name: 'Alice', role: 'admin', apiKey: 'demo' } | |
| context.log.set({ auth: { ok: true, userId: user.id, role: user.role } }) | |
| return next({ | |
| context: { ...context, user }, | |
| }) | |
| }) | |
| const authed = base.use(({ context, next }) => { | |
| const user: AuthedUser = { id: 'u-1', name: 'Alice', role: 'admin', apiKey: 'demo' } | |
| context.log.set({ auth: { ok: true, userId: user.id, role: user.role } }) | |
| return next({ | |
| context: { ...context, user }, | |
| }) | |
| }) |
🧰 Tools
🪛 ESLint
[error] 49-49: Async arrow function has no 'await' expression.
(require-await)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/orpc/src/index.ts` around lines 49 - 55, The middleware `authed` is
declared `async` but doesn't `await`, causing a `require-await` lint error;
remove the `async` modifier on the arrow function passed to `base.use` (i.e.,
change `async ({ context, next }) => { ... }` to `({ context, next }) => { ...
}`) while keeping the body intact (creating the `AuthedUser`, calling
`context.log.set`, and returning `next({ context: { ...context, user } })`) so
the function still returns the Promise from `next`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
examples/orpc/src/index.ts (1)
62-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove unnecessary
asyncfrom middleware (require-await).The middleware returns
next(...)directly and does not await anything, so this triggers lint failure.✅ Minimal fix
-const authed = base.use(async ({ context, next }) => { +const authed = base.use(({ context, next }) => { const user: AuthedUser = { id: 'u-1', name: 'Alice', role: 'admin', apiKey: 'demo' } context.log.set({ auth: { ok: true, userId: user.id, role: user.role } }) return next({ context: { ...context, user }, }) })#!/bin/bash # Verify there are no base.use async middlewares without await in this file. rg -nP "base\.use\(async\s*\(\{[^)]*\}\)\s*=>" examples/orpc/src/index.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/orpc/src/index.ts` around lines 62 - 68, The middleware "authed" passed to base.use is declared async but never awaits, causing a require-await lint error; change the declaration to a synchronous function by removing the async keyword from the authed middleware (i.e., update the base.use handler signature that currently reads "async ({ context, next }) =>" to a plain "({ context, next }) =>"), leaving the body and the return of next({ context: { ...context, user } }) unchanged so behavior is preserved.examples/orpc/src/ui.ts (1)
209-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape dynamic JSON tokens before writing highlighted output to
innerHTML.Response data is interpolated into HTML spans without escaping, so HTML/script payloads in response strings can execute in the page.
🔒 Minimal fix
+ function escapeHtml(value) { + return value + .replace(/&/g, '&') + .replace(/</g, '<') + .replace(/>/g, '>') + .replace(/"/g, '"') + .replace(/'/g, ''') + } + function syntaxHighlight(json) { return JSON.stringify(json, null, 2) .replace(/("(\\\\u[a-fA-F0-9]{4}|\\\\[^u]|[^\\\\"])*")(\\s*:)?/g, (match, str, _, colon) => { - if (colon) return '<span class="key">' + str + '</span>' + colon - return '<span class="string">' + str + '</span>' + const safe = escapeHtml(str) + if (colon) return '<span class="key">' + safe + '</span>' + colon + return '<span class="string">' + safe + '</span>' }) .replace(/\\b(true|false)\\b/g, '<span class="boolean">$1</span>') .replace(/\\bnull\\b/g, '<span class="null">null</span>') .replace(/\\b(-?\\d+\\.?\\d*([eE][+-]?\\d+)?)\\b/g, '<span class="number">$1</span>') }Also applies to: 252-252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/orpc/src/ui.ts` around lines 209 - 218, The syntaxHighlight function builds HTML with unescaped JSON pieces, enabling XSS when its return is assigned to innerHTML; fix by HTML-escaping dynamic tokens before wrapping them in spans (create/ use an escapeHtml helper and apply it to the captured string token `str` in the first replace and to the captured groups used for numbers/booleans/null in the other replaces), so the replacement callbacks use the escaped value rather than raw match; ensure you escape quotes, ampersands, <, > and slashes when producing the span contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/orpc/src/ui.ts`:
- Around line 244-257: The code currently assumes every response is JSON (using
await res.json()) and treats parse failures as network errors; change the logic
so after receiving res you check the Content-Type (e.g.,
res.headers.get('content-type')) and if it contains "application/json" parse
with res.json() and render via syntaxHighlight(data), otherwise use await
res.text() and render the plain text/HTML into el.body (and set an appropriate
class like 'response-body text' or similar); ensure only real network exceptions
fall into the catch block and preserve updating el.header, el.status,
el.methodPath, and el.timing for non-JSON responses as well (references:
variables/res object, el, syntaxHighlight, method, path, ms).
---
Duplicate comments:
In `@examples/orpc/src/index.ts`:
- Around line 62-68: The middleware "authed" passed to base.use is declared
async but never awaits, causing a require-await lint error; change the
declaration to a synchronous function by removing the async keyword from the
authed middleware (i.e., update the base.use handler signature that currently
reads "async ({ context, next }) =>" to a plain "({ context, next }) =>"),
leaving the body and the return of next({ context: { ...context, user } })
unchanged so behavior is preserved.
In `@examples/orpc/src/ui.ts`:
- Around line 209-218: The syntaxHighlight function builds HTML with unescaped
JSON pieces, enabling XSS when its return is assigned to innerHTML; fix by
HTML-escaping dynamic tokens before wrapping them in spans (create/ use an
escapeHtml helper and apply it to the captured string token `str` in the first
replace and to the captured groups used for numbers/booleans/null in the other
replaces), so the replacement callbacks use the escaped value rather than raw
match; ensure you escape quotes, ampersands, <, > and slashes when producing the
span contents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 27675694-6843-47e3-88c1-43fbfca706e1
📒 Files selected for processing (3)
apps/docs/content/3.integrate/frameworks/15.orpc.mdexamples/orpc/src/index.tsexamples/orpc/src/ui.ts
Comment on lines
+244
to
+257
| const data = await res.json() | ||
|
|
||
| el.header.style.display = 'flex' | ||
| el.status.textContent = res.status | ||
| el.status.className = 'status status-' + (res.status < 300 ? '2xx' : res.status < 500 ? '4xx' : '5xx') | ||
| el.methodPath.textContent = method + ' ' + path | ||
| el.timing.textContent = ms + 'ms' | ||
| el.body.className = 'response-body' | ||
| el.body.innerHTML = syntaxHighlight(data) | ||
| } catch (err) { | ||
| el.header.style.display = 'none' | ||
| el.body.className = 'response-body' | ||
| el.body.textContent = 'Network error: ' + err.message | ||
| } |
Contributor
There was a problem hiding this comment.
Handle non-JSON responses explicitly instead of reporting them as “Network error”.
Line 244 assumes JSON for every response. A valid text/HTML error body (e.g., 404 text) throws during parsing and is misclassified in the catch branch.
✅ Minimal fix
- const data = await res.json()
+ const contentType = res.headers.get('content-type') || ''
+ const data = contentType.includes('application/json')
+ ? await res.json()
+ : await res.text()
...
- el.body.innerHTML = syntaxHighlight(data)
+ if (typeof data === 'string') {
+ el.body.textContent = data
+ } else {
+ el.body.innerHTML = syntaxHighlight(data)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const data = await res.json() | |
| el.header.style.display = 'flex' | |
| el.status.textContent = res.status | |
| el.status.className = 'status status-' + (res.status < 300 ? '2xx' : res.status < 500 ? '4xx' : '5xx') | |
| el.methodPath.textContent = method + ' ' + path | |
| el.timing.textContent = ms + 'ms' | |
| el.body.className = 'response-body' | |
| el.body.innerHTML = syntaxHighlight(data) | |
| } catch (err) { | |
| el.header.style.display = 'none' | |
| el.body.className = 'response-body' | |
| el.body.textContent = 'Network error: ' + err.message | |
| } | |
| const contentType = res.headers.get('content-type') || '' | |
| const data = contentType.includes('application/json') | |
| ? await res.json() | |
| : await res.text() | |
| el.header.style.display = 'flex' | |
| el.status.textContent = res.status | |
| el.status.className = 'status status-' + (res.status < 300 ? '2xx' : res.status < 500 ? '4xx' : '5xx') | |
| el.methodPath.textContent = method + ' ' + path | |
| el.timing.textContent = ms + 'ms' | |
| el.body.className = 'response-body' | |
| if (typeof data === 'string') { | |
| el.body.textContent = data | |
| } else { | |
| el.body.innerHTML = syntaxHighlight(data) | |
| } | |
| } catch (err) { | |
| el.header.style.display = 'none' | |
| el.body.className = 'response-body' | |
| el.body.textContent = 'Network error: ' + err.message | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/orpc/src/ui.ts` around lines 244 - 257, The code currently assumes
every response is JSON (using await res.json()) and treats parse failures as
network errors; change the logic so after receiving res you check the
Content-Type (e.g., res.headers.get('content-type')) and if it contains
"application/json" parse with res.json() and render via syntaxHighlight(data),
otherwise use await res.text() and render the plain text/HTML into el.body (and
set an appropriate class like 'response-body text' or similar); ensure only real
network exceptions fall into the catch block and preserve updating el.header,
el.status, el.methodPath, and el.timing for non-JSON responses as well
(references: variables/res object, el, syntaxHighlight, method, path, ms).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/evlog/test/frameworks/orpc.test.ts`:
- Around line 412-424: The keep callback in the test 'lets the keep callback
force-keep based on context' is reading ctx.context.pinged but the procedure
sets pinged via context.log.set which populates ctx.event.pinged; update the
mockImplementation for keep to check ctx.event.pinged (and set ctx.shouldKeep =
true when present) so the force-keep branch is actually exercised (refer to the
mocked keep function and the test case name to locate the change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 13d5a153-78ae-47f4-b1f6-6dcb92d6be95
📒 Files selected for processing (4)
apps/docs/content/3.integrate/frameworks/15.orpc.mdexamples/orpc/src/index.tspackages/evlog/src/orpc/index.tspackages/evlog/test/frameworks/orpc.test.ts
Comment on lines
+412
to
+424
| it('lets the keep callback force-keep based on context', async () => { | ||
| const { drain, keep } = createPipelineSpies() | ||
| keep.mockImplementation((ctx) => { | ||
| if (ctx.context.pinged) ctx.shouldKeep = true | ||
| }) | ||
| const { client } = buildClient({ drain, keep }) | ||
|
|
||
| await client.ping({}) | ||
| await waitForDrainCalls(drain) | ||
|
|
||
| expect(keep).toHaveBeenCalledOnce() | ||
| expect(drain).toHaveBeenCalledOnce() | ||
| }) |
Contributor
There was a problem hiding this comment.
Test checks wrong property for force-keep behavior.
The keep callback checks ctx.context.pinged, but the procedure writes context.log.set({ pinged: true }) which populates ctx.event.pinged, not ctx.context.pinged. The test passes because both keep and drain are called once, but the conditional force-keep logic isn't actually being exercised.
🧪 Proposed fix
it('lets the keep callback force-keep based on context', async () => {
const { drain, keep } = createPipelineSpies()
keep.mockImplementation((ctx) => {
- if (ctx.context.pinged) ctx.shouldKeep = true
+ if (ctx.event.pinged) ctx.shouldKeep = true
})
const { client } = buildClient({ drain, keep })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('lets the keep callback force-keep based on context', async () => { | |
| const { drain, keep } = createPipelineSpies() | |
| keep.mockImplementation((ctx) => { | |
| if (ctx.context.pinged) ctx.shouldKeep = true | |
| }) | |
| const { client } = buildClient({ drain, keep }) | |
| await client.ping({}) | |
| await waitForDrainCalls(drain) | |
| expect(keep).toHaveBeenCalledOnce() | |
| expect(drain).toHaveBeenCalledOnce() | |
| }) | |
| it('lets the keep callback force-keep based on context', async () => { | |
| const { drain, keep } = createPipelineSpies() | |
| keep.mockImplementation((ctx) => { | |
| if (ctx.event.pinged) ctx.shouldKeep = true | |
| }) | |
| const { client } = buildClient({ drain, keep }) | |
| await client.ping({}) | |
| await waitForDrainCalls(drain) | |
| expect(keep).toHaveBeenCalledOnce() | |
| expect(drain).toHaveBeenCalledOnce() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/evlog/test/frameworks/orpc.test.ts` around lines 412 - 424, The keep
callback in the test 'lets the keep callback force-keep based on context' is
reading ctx.context.pinged but the procedure sets pinged via context.log.set
which populates ctx.event.pinged; update the mockImplementation for keep to
check ctx.event.pinged (and set ctx.shouldKeep = true when present) so the
force-keep branch is actually exercised (refer to the mocked keep function and
the test case name to locate the change).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
Closes #297
📚 Description
Adds
evlog/orpcso each oRPC procedure call becomes a single wide event. Two complementary primitives that mirror oRPC's two layers:withEvlog(handler)— wraps any handler exposing.handle(request, options)from@orpc/server/fetch(RPCHandler,OpenAPIHandler, custom). The wrapped handler is a transparentProxy, so it stays an instance of the original (wrapped instanceof RPCHandler === true). Each matched request creates a request-scoped logger, injects it ascontext.loginto the oRPC initial context, and emits one wide event with the standard pipeline (drain,enrich,keep,include/exclude,routes, plugins). Excluded routes still receive a no-op logger so procedures never crash on missing fields.evlog()— procedure middleware (os.use(evlog())). Tags the wide event withoperation(the procedure path joined with.), forwards the request logger ascontext.log, and promotes the level toerrorwhen a procedure throws — before re-throwing so oRPC's ownORPCErrorpath runs untouched.useLogger()is exposed for off-context access (utility modules, deep service functions).EvlogOrpcContextplugs intoos.$context()for typed access.Followed
.agents/skills/create-framework-integration/SKILL.md— all 16 touchpoints addressed: source, build config, exports, peer dep, 18 integration tests, framework docs page (3.integrate/frameworks/15.orpc.md), overview / installation / landing /FeatureFrameworksupdates, README + Framework Support table,review-logging-patternsskill update, runnable example app atexamples/orpc/(Bun +OpenAPIHandler+ PostHog drain + test UI), rootexample:orpcscript, and theorpcscope added tosemantic-pull-request.yml+ the PR template.📝 Checklist
Summary by CodeRabbit
New Features
Examples
Documentation
Tests
Chores